-
Notifications
You must be signed in to change notification settings - Fork 280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scripts: Add guestagent and trivy into wsl-distro tarball instead of installing at runtime #7145
Conversation
This allows dependencies to depend on other dependencies, including across platforms; we intend to allow WSLDistro to depend on Linux bits. Signed-off-by: Mark Yen <[email protected]>
This makes the go utilities act like dependencies, so we can use them as dependencies of other dependencies. Signed-off-by: Mark Yen <[email protected]>
This embeds rancher-desktop-guestagent as part of the WSL distro on post- install, so that we do not need to copy it into the VM at run time. Signed-off-by: Mark Yen <[email protected]>
Signed-off-by: Mark Yen <[email protected]>
- We don't actually support localization - There is no point in including source maps, we don't ship the sources - Support merging file lists Signed-off-by: Mark Yen <[email protected]>
const outFile = this.outFile(context); | ||
|
||
console.log(`Building go utility \x1B[1;33;40m${ this.name }\x1B[0m from ${ sourceDir } to ${ outFile }...`); | ||
await simpleSpawn('go', ['build', '-ldflags', '-s -w', '-o', outFile, '.'], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to handle any potential errors here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any errors that return a non-zero exit code would throw an exception here, so it should be fine.
|
||
async download(context: DownloadContext): Promise<void> { | ||
// Build the extension proxy image. | ||
const workDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'rd-build-rdx-pf-')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the fs.promise
methods also need error handling, I am just confirming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because they throw exceptions (which will fall over correctly).
const { size } = await fs.promises.stat(fromPath); | ||
const inputFile = fs.createReadStream(fromPath); | ||
|
||
console.log(`WSL Distro: Adding ${ fromPath } to ${ name }...`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit; I noticed on line 172 it is spelled WSLDistro
and here is WSL Distro
, we should probably just use one of them to unify the logs.
scripts/dependencies/go-source.ts
Outdated
|
||
override environment(context: DownloadContext) { | ||
return { | ||
...super.environment(context), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if we need GOOS
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super.environment()
(line 33) sets GOOS
.
scripts/package.ts
Outdated
@@ -127,6 +131,23 @@ class Builder { | |||
break; | |||
} | |||
|
|||
// If we have files (resp. extraFiles / extraResources) both at the top |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be clearer to change this to
// When there are files (e.g., extraFiles or extraResources) specified at both
// the top-level and platform-specific levels, we need to combine them
// and place the combined list at the top level. This approach enables us to have
// platform-specific exclusions, since the two lists are initially processed
// separately and then merged together afterward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll just grab your text wholesale.
This rewords the comment describing how we need to merge the files lists for electron-builder because they way they do merging is a union of the two resultant sets instead of calculating a set based on a union of the rules. Signed-off-by: Mark Yen <[email protected]>
This should make it easier to handle custom paths / subcommands. For example, we may want: new goUtils.GoDependency('networking/cmd/vm', { outputPath: 'staging/vm-switch'}) Signed-off-by: Mark Yen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated GoDependency
to better support multiple commands in one package; see the last commit message for details.
const outFile = this.outFile(context); | ||
|
||
console.log(`Building go utility \x1B[1;33;40m${ this.name }\x1B[0m from ${ sourceDir } to ${ outFile }...`); | ||
await simpleSpawn('go', ['build', '-ldflags', '-s -w', '-o', outFile, '.'], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any errors that return a non-zero exit code would throw an exception here, so it should be fine.
scripts/dependencies/go-source.ts
Outdated
|
||
override environment(context: DownloadContext) { | ||
return { | ||
...super.environment(context), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super.environment()
(line 33) sets GOOS
.
|
||
async download(context: DownloadContext): Promise<void> { | ||
// Build the extension proxy image. | ||
const workDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'rd-build-rdx-pf-')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because they throw exceptions (which will fall over correctly).
scripts/package.ts
Outdated
@@ -127,6 +131,23 @@ class Builder { | |||
break; | |||
} | |||
|
|||
// If we have files (resp. extraFiles / extraResources) both at the top |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll just grab your text wholesale.
Fixes #7137.
Dependency
s, and implement crude dependency tracking so that they can depend on each other.GoDependency
can take an array as the first argument, sonew GoDependency(['networking', 'cmd', 'network'])
is valid.staging
directory instead so not packaging them is easier).